Skip to content

Avoid listing models for explicit text model IDs#231

Open
chubes4 wants to merge 2 commits into
WordPress:trunkfrom
chubes4:explicit-model-no-list
Open

Avoid listing models for explicit text model IDs#231
chubes4 wants to merge 2 commits into
WordPress:trunkfrom
chubes4:explicit-model-no-list

Conversation

@chubes4

@chubes4 chubes4 commented May 2, 2026

Copy link
Copy Markdown

Summary

  • Adds a provider-overridable path for explicit model IDs to provide metadata without listing all provider models first.
  • Uses that path for OpenAI-compatible text-generation model families, while non-text IDs such as gpt-image-* still fall back to listed metadata.
  • Keeps cached/listed metadata authoritative when it is already available.

Fixes #230.

Why

Explicit provider/model selection currently calls the provider model-list endpoint before the model can be instantiated. For OpenAI-compatible providers, that means getProviderModel( 'openai', 'gpt-5.4' ) can fail on a transient GET /models issue before the actual generation endpoint has a chance to validate the explicitly requested model.

Testing

  • composer test -- --filter 'GetProviderModelWithExplicitOpenAiModelIdDoesNotListModels|GetModelMetadataForExplicitModelIdDoesNotListModels|GetModelMetadataForNonTextExplicitModelIdListsModels|GetModelMetadataUsesCachedListedMetadataWhenAvailable'
  • composer test currently runs all 1091 tests locally, but exits non-zero on PHP 8.5 because existing reflection tests emit deprecation output and PHPUnit marks 83 tests risky. No assertion failures were reported.

AI assistance

  • AI assistance: Yes
  • Tool(s): OpenCode (GPT-5.5)
  • Used for: Drafting the patch, tests, and PR description; Chris reviewed the behavior on a local Studio site and remains responsible for the change.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @felixarntz.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: felixarntz.

Co-authored-by: chubes4 <extrachill@git.wordpress.org>
Co-authored-by: JasonTheAdams <jason_the_adams@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.72%. Comparing base (6317042) to head (123286e).
⚠️ Report is 3 commits behind head on trunk.

Additional details and impacted files
@@             Coverage Diff              @@
##              trunk     #231      +/-   ##
============================================
- Coverage     88.12%   87.72%   -0.41%     
- Complexity     1213     1217       +4     
============================================
  Files            60       60              
  Lines          3934     4121     +187     
============================================
+ Hits           3467     3615     +148     
- Misses          467      506      +39     
Flag Coverage Δ
unit 87.72% <100.00%> (-0.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The OpenAI-compatible abstraction is about HTTP/JSON shape, not OpenAI's
model namespace. Drop the gpt-/o3/dall-e prefix gating and the synthetic
text-generation metadata override; the per-provider override belongs in
ai-provider-for-openai (and similar repos for other compatible providers).

The generic createModelMetadataForExplicitModelId() hook on
AbstractApiBasedModelMetadataDirectory is unchanged so providers can still
opt in.

@felixarntz felixarntz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chubes4 While this addresses the reported concern, it creates data inconsistencies when considering the API surface of this class holistically.

I think there's a path forward, but it requires some modifications in the approach.

Comment on lines +78 to +81
$explicitModelMetadata = $this->createModelMetadataForExplicitModelId($modelId);
if ($explicitModelMetadata !== null) {
return $explicitModelMetadata;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit of a strange design that this will only be used if no cached value has been found, especially since this injected model metadata does not get cached itself. what if the data from the API differs from what's locally defined in the code?

Either we need to cache the result of this too (which I'd have other concerns over though because of cache invalidation) or we need to always run this before the cache logic. If the implementer wants to add some kind of caching themselves for it, they still could.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the data from the API differs from what's locally defined in the code?

I think we should run it before cache logic. I'm trying to keep this as simple as possible.

Comment on lines 69 to +78
final public function getModelMetadata(string $modelId): ModelMetadata
{
if ($this->hasCache(self::MODELS_CACHE_KEY)) {
$modelsMetadata = $this->getModelMetadataMap();
if (isset($modelsMetadata[$modelId])) {
return $modelsMetadata[$modelId];
}
}

$explicitModelMetadata = $this->createModelMetadataForExplicitModelId($modelId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being only in getModelMetadata() but not in listModelMetdata() will lead to inconsistent data - so the approach doesn't work correctly in terms of the full API.

I understand this is meant to be used to bypass the need for a request in case an explicit model ID is passed, but that's not controlled by this class. Any code could call getModelMetadata() here, so it'd be confusing if getModelMetadata() returned a different set of metadata than the same model would appear with as part of listModelMetadata().

I think there's a way to implement this in a way that the code here still bypasses the request while ensuring consistent data. E.g. for listModelMetadata() we could make the request first (necessary there!), and then for every single model ID call this method. If the method returns something, override what the API returned - this way it's the same result as here.

There's performance and data integrity to consider on that path - we'd probably want to build in memoization and potentially even caching of this so that:

  • this isn't called for every model every time that listModelMetadata() is called
  • this doesn't create multiple instances for the same model ID within the same request

It's not super trivial, but I think it's possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review and thoughtful comments!

I think this is mainly a problem on slow connections or local environments where the CPU/RAM is constrained. I have not seen it on my live sites but have seen it repeatedly in a local environment inside Studio/WordPress Playground.

I will revisit this based on your feedback and update the PR to consider the broader implications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Explicit model instantiation should not require live model-list metadata fetch

3 participants